-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update cancellation token handling behavior for certain analyzer APIs #69108
Update cancellation token handling behavior for certain analyzer APIs #69108
Conversation
@@ -26,7 +27,6 @@ public class CompilationWithAnalyzers | |||
private readonly ImmutableArray<DiagnosticAnalyzer> _analyzers; | |||
private readonly ImmutableArray<DiagnosticSuppressor> _suppressors; | |||
private readonly CompilationWithAnalyzersOptions _analysisOptions; | |||
private readonly CancellationToken _cancellationToken; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad captured CT.
public CancellationToken CancellationToken => _cancellationToken; | ||
[Obsolete("This CancellationToken is always 'default'", error: false)] | ||
[EditorBrowsable(EditorBrowsableState.Never)] | ||
public CancellationToken CancellationToken => default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kept for binary compat. but if anyone is using this, they're warned to stop.
public CompilationWithAnalyzers(Compilation compilation, ImmutableArray<DiagnosticAnalyzer> analyzers, AnalyzerOptions? options, CancellationToken cancellationToken) | ||
: this(compilation, analyzers, options) | ||
{ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kept for binary compat. if anyone is using this, they're warning to stop.
@@ -225,15 +228,16 @@ private void VerifyAdditionalFile(AdditionalText file) | |||
/// <summary> | |||
/// Returns diagnostics produced by all <see cref="Analyzers"/>. | |||
/// </summary> | |||
[EditorBrowsable(EditorBrowsableState.Never)] | |||
public Task<ImmutableArray<Diagnostic>> GetAnalyzerDiagnosticsAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't warn on this, as we often have APIs where user provided cancellationtokens are optional, and we want to allow users to call those without a CT for simple programming purposes. We do hide this though, so that the user only sees the method below, which follows our standard api pattern of an actually 'optional' parameter.
/// <inheritdoc cref="WithAnalyzers(Compilation, ImmutableArray{DiagnosticAnalyzer}, AnalyzerOptions?)"/> | ||
[Obsolete("Use WithAnalyzers overload without a cancellation token", error: false)] | ||
[EditorBrowsable(EditorBrowsableState.Never)] | ||
public static CompilationWithAnalyzers WithAnalyzers(this Compilation compilation, ImmutableArray<DiagnosticAnalyzer> analyzers, AnalyzerOptions? options, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kept for binary compat. if anyone is using this, they're warned to stop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Consider adding this to the banned APIs in dotnet/roslyn-analyzers so it applies even to users who reference old versions of the compiler in their analyzer projects
@dotnet/roslyn-compiler ptal. |
src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs
Outdated
Show resolved
Hide resolved
/// <summary> | ||
/// Returns a new compilation with attached diagnostic analyzers. | ||
/// </summary> | ||
/// <param name="compilation">Compilation to which analyzers are to be added.</param> | ||
/// <param name="analyzers">The set of analyzers to include in future analyses.</param> | ||
/// <param name="options">Options that are passed to analyzers.</param> | ||
/// <param name="cancellationToken">A cancellation token that can be used to abort analysis.</param> | ||
public static CompilationWithAnalyzers WithAnalyzers(this Compilation compilation, ImmutableArray<DiagnosticAnalyzer> analyzers, AnalyzerOptions? options = null, CancellationToken cancellationToken = default) | ||
#pragma warning disable RS0027 // API with optional parameter(s) should have the most parameters amongst its public overloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Consider using SuppressMessage
for this one since it has a space specifically for the justification
src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs
Show resolved
Hide resolved
…Analyzers.cs Co-authored-by: Sam Harwell <[email protected]>
…Analyzers.cs Co-authored-by: Sam Harwell <[email protected]>
…Analyzers.cs Co-authored-by: Sam Harwell <[email protected]>
I submitted dotnet/roslyn-sdk#1107 as a supporting change |
@dotnet/roslyn-compiler ptal. |
@333fred @RikkiGibson can you ptal? |
using System.Threading; | ||
|
||
namespace Microsoft.CodeAnalysis.Diagnostics | ||
{ | ||
public static class DiagnosticAnalyzerExtensions | ||
{ | ||
/// <inheritdoc cref="WithAnalyzers(Compilation, ImmutableArray{DiagnosticAnalyzer}, AnalyzerOptions?)"/> | ||
[Obsolete("Use WithAnalyzers overload without a cancellation token", error: false)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're probably okay since this is just a warning, but let's confirm that we don't need to document this as a break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tagging @333fred, but i'm virtually certain we don't need to document. There is no actual 'break' here. Both binary and source bulds will continue to be fine. Just that source builds may get a warning. We obsolete things like that all the time without documenting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (iteration 6)
Fixes #41522.
The old API was busted (and never used) where a cancellation token was provided at creation, and then potentially passed along if you called particular methods. This was highly unsafe as said times had no relation to each other, and you could provide a cancellation token at one point unrelated to the one used to compute diagnostics.
We fixed all our own usages everywhere to do the right thing. Specifically, to not pass a cancellation token to the constructor, and to always pass a correct one to the GetXXXDiagnostics methods. This PR just goes and adds appropriate overloads, obsolete warnings, and editorbrowsable hidings, to ensure that any other consumers do the right thing as well.
This change is entirely binary compatible with existing releases, using our standard patterns for preserving (but hiding) bad apis, and introducing new APIs that will be preferred in source.